Add script to count unique learnersin Sensei #2172
Conversation
|
@jonathanbossenger This pull request is ready for a review! 🥳 I'll note, I suppressed a lint error in line 195: Jonathan and I discussed that this might be permissible in this case. |
digitalchild
added
[Dev] Needs Review
| $query .= " AND comment_date_gmt <= '" . $to_date->format( 'Y-m-d H:i:s' ) . "'"; | ||
| } | ||
|
|
||
| $student_count = $wpdb->get_var( $query ); //phpcs:ignore WordPress.DB.PreparedSQL.NotPrepared |
There was a problem hiding this comment.
@pkevan Ben and I discussed this, and we feel ignoring the PHPCS rule here is ok, but we'd appreciate your opinion. I personally can't see how this could be attacked via SQL injection, but maybe I'm missing something.
There was a problem hiding this comment.
While it's true that the current code isn't vulnerable, that doesn't mean in the future it wouldn't be, so it's preferable to built in elements which would force it to not be.
For example, the $from_date variable assignment logic could be changed to grab from the $_GET variable directly or the appended $query concatenation logic changed in a way which did make it.
By adding some prepare statements, it would help avoid those scenarios, particularly when dealing with user input.
There was a problem hiding this comment.
@pkevan Thanks, that's a fair point, I will work on adding sanitization to the date variables, and change the SQL concatenation to use a prepare statement.
|
@pkevan when you have a moment, @bsanevans and I would appreciate a second pair of eyes on this PR, thanks. |
|
@bsanevans I have merged #2178, so I am going to close this out. |
Adds the script provided by @m1r0 in #2111 to wporg-learn plugin